🤖 Kimi Code Review

Security & Dependencies

• Critical: Cargo.lock shows num-bigint downgraded from 0.4.6 → 0.3.3 in mt-field and mt-koala-bear (lines 4832, 4856). Version 0.3.3 is from 2020 and lacks security fixes present in 0.4.x. Verify this is intentional and does not reintroduce known vulnerabilities (e.g., potential DoS via crafted inputs).

• Portability: New transitive dependency objc2-foundation (line 5236) appears in rec_aggregation. This is macOS-specific; confirm it doesn't break Linux-only deployments or add unnecessary bloat.

Consensus & API Safety

• Breaking Change: AggregatedXMSS::deserialize renamed to decompress and serialize to compress (lines 211, 220, 255). If this changes the wire format, it requires network coordination (hard fork) to avoid consensus splits between old/new nodes.

• Error Handling: Changing xmss_aggregate from panicking to returning Result (lines 110, 165, 195) is correct for consensus safety, but verify all call sites in ethlambda properly handle the new AggregationError::Aggregation variant rather than expecting panics.

Code Quality

• crates/common/crypto/src/lib.rs:46-48: The #[from] LeanAggregationError implementation is idiomatic and properly propagates upstream errors.

• crates/common/crypto/src/lib.rs:110,165,195: The ? operator is correctly applied to xmss_aggregate calls, converting the new error type appropriately.

• Documentation update at lines 121-125 correctly changes # Panics to # Errors to reflect the new non-panicking behavior.

Minor

• Ensure the include_dir crate (added to lean_compiler) doesn't embed sensitive build-time paths into the binary if reproducible builds are required.

Recommendation: Before merging, audit the leanMultisig diff at f66d4a9 to confirm the num-bigint downgrade is justified and that compress/decompress maintain cryptographic compatibility with existing proofs on devnet-4.

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

🤖 Claude Code Review

Here is the full review output:

PR #400: chore(deps): bump leanMultisig to f66d4a9

Overview

This PR bumps lean-multisig and leansig_wrapper from 5eba3b1 to f66d4a9 and adapts ethlambda-crypto to three upstream API changes:

• AggregatedXMSS::serialize() → compress()
• AggregatedXMSS::deserialize() → decompress()
• xmss_aggregate(...) now returns Result<(Vec, AggregatedXMSS), AggregationError> instead of panicking

The changes are clean and minimal. Correctness and style are solid.

Code Quality

Error handling improvement (src/lib.rs:107, 162, 192): Converting three xmss_aggregate(...) call sites from implicit panics to ?-propagation is the most substantive change, and it is done correctly. The upstream LeanAggregationError is wrapped via #[from] in the new AggregationError::Aggregation variant — idiomatic and clean.

Doc comment update (src/lib.rs around aggregate_mixed): The # Panics section is correctly replaced with # Errors with accurate description of the new failure mode. Good.

Rename consistency: All four sites (deserialize_children, serialize_aggregate, verify_aggregated_signature, and the two-line serialize_aggregate helper) switch together. No stale call sites left.

_sorted_pubkeys discarded: This was already the case before the PR. The sorted validator set is silently dropped at every aggregation call site. That pre-existing pattern is unchanged here, but it means ethlambda never asserts that the returned set matches expectations. Not a regression, just worth a future ticket.

New Transitive Dependencies (Cargo.lock)

Three categories of new transitive deps from the upstream bump deserve attention:

1. objc2 and objc2-foundation 0.3.2 — pulled in by rec_aggregation. These are macOS Objective-C framework bindings. On Linux this typically compiles cleanly (stubs/no-ops), but macOS-specific bindings appearing inside a ZK aggregation library is unexpected. It is worth confirming upstream why rec_aggregation needs macOS framework access. If it relates to hardware memory detection (see system-info below), that is probably harmless, but it adds platform-specific code to the cryptographic critical path.

2. num-bigint downgrade from 0.4.6 → 0.3.3 — mt-field and mt-koala-bear now use the older series. num-bigint 0.3.x predates several performance improvements and API additions made in 0.4. The downstream impact on proof generation speed or correctness inside the ZK backend is opaque from this diff alone. This is not a regression introduced by ethlambda, but it is a signal worth tracking upstream.

3. zk-alloc and system-info — new crates from leanMultisig. system-info + libc + rayon suggests runtime CPU/memory probing for ZK parallelism tuning. zk-alloc with libc + system-info likely wraps a custom allocator. If zk-alloc registers a global allocator it would affect the entire ethlambda process. This is only visible by reading the upstream crate — confirming it does not install a #[global_allocator] is a worthwhile check before merging.

Testing Gap

The PR description accurately notes that all crypto integration tests are #[ignore] due to slow leanVM ops. This means the three changed xmss_aggregate call sites — and in particular the new error path — are not exercised in CI. The new AggregationError::Aggregation variant has no unit test. For a cryptographic aggregation function this is pre-existing debt, but it becomes slightly more pressing now that there is a new observable failure mode. A cheap addition would be a test that passes a deliberately invalid child proof and asserts the error is returned rather than a panic.

Summary

The ethlambda-side changes are correct, minimal, and idiomatic. The main open question is upstream: confirm that zk-alloc does not install a global allocator, and understand why rec_aggregation pulls in macOS ObjC bindings. Neither is a blocker if the upstream crate is trusted, but both are worth verifying before the bump lands on a release branch.

Automated review by Claude (Anthropic) · sonnet · custom prompt

🤖 Codex Code Review

Findings:

1. High: this PR changes the AggregatedSignatureProof.proof_data wire/storage encoding from raw serialize() bytes to compress() bytes, but the read path now only accepts the new format. serialize_aggregate() now writes compressed bytes at crates/common/crypto/src/lib.rs, while both child-proof loading and network/block verification only call decompress() at crates/common/crypto/src/lib.rs and crates/common/crypto/src/lib.rs. If legacy proofs already exist in the DB, mempool, fixtures, or on the network, they become unreadable immediately after upgrade. In consensus code, that needs an explicit migration/fork gate or a dual-format decoder during the transition.

2. High: the ByteListMiB bound is now enforced on the compressed representation only, not on the actual in-memory proof size. The SSZ type still advertises a 1 MiB cap at crates/common/types/src/block.rs, but serialize_aggregate() checks aggregate.compress() length at crates/common/crypto/src/lib.rs, and the verifier inflates attacker-controlled bytes with decompress() at crates/common/crypto/src/lib.rs. That weakens the previous resource bound: a proof whose compressed form is under 1 MiB but whose expanded tree is much larger can now be accepted and force extra CPU/memory during verification. For a blockchain client, that is a meaningful DoS surface unless lean-multisig enforces a strict uncompressed-size cap internally.

The error propagation change from panic-prone aggregation to Result-based handling looks like an improvement.

I couldn’t compile this locally because the new leanMultisig git rev is not available offline in this environment, so I was not able to inspect or execute compress()/decompress() directly.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Greptile Summary

Bumps the lean-multisig and leansig_wrapper git dependencies from 5eba3b1 to f66d4a9 and adapts the three changed public API methods (serialize→compress, deserialize→decompress, and xmss_aggregate now returns Result).

• aggregate_signatures, aggregate_mixed, and aggregate_proofs in lib.rs now propagate xmss_aggregate errors via ? instead of panicking, backed by a new Aggregation(#[from] LeanAggregationError) variant on AggregationError. The public API surface of ethlambda-crypto is unchanged.
• The lock file picks up several new transitive crates (include_dir, system-info, zk-alloc, objc2-foundation), a num-bigint downgrade from 0.4.6 → 0.3.3 in the mt-field/mt-koala-bear sub-crates, and new serde/tracing dependencies in upstream provers.

Confidence Score: 4/5

The change is a focused dependency bump with a clean, mechanical API adaptation; the core aggregation and verification paths are structurally unchanged.

All three aggregation functions correctly swap the panic path for ?-propagated errors, and the rename of serialize/deserialize to compress/decompress is applied consistently at every call site. The only minor gaps are a doc comment that describes only one of the two conditions under which AggregationError::Aggregation can be returned, and a new macOS Objective-C transitive dependency (objc2-foundation) worth verifying on Linux CI targets.

The Cargo.lock entry for rec_aggregation is worth a second look due to the newly added objc2/objc2-foundation dependencies.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[aggregate_signatures / aggregate_mixed / aggregate_proofs] --> B[xmss_aggregate]
    B -->|Ok| C[serialize_aggregate → compress]
    B -->|Err LeanAggregationError| D[AggregationError::Aggregation via #from]
    E[deserialize_children] --> F[AggregatedXMSS::decompress]
    F -->|None| G[AggregationError::ChildDeserializationFailed]
    F -->|Some| H[AggregatedXMSS value]
    H --> B
    C --> I[ByteListMiB returned to caller]
    D --> J[Result::Err returned to caller]
    G --> J

Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/common/crypto/src/lib.rs:124-127
**Doc comment covers only one of two failure modes for `xmss_aggregate`**

The updated `# Errors` section says the `Aggregation` error fires when a *deserialized child proof* is cryptographically invalid, but `xmss_aggregate` can also return an error when *raw XMSS signatures* are invalid (e.g. signed against a different message or slot). A caller who relies solely on the doc comment to understand what triggers `AggregationError::Aggregation` would have an incomplete picture, which could matter when deciding how to handle or log the error at the call site.

### Issue 2 of 2
Cargo.lock:6378-6396
**New macOS-only transitive dependencies pulled in via `rec_aggregation`**

`objc2-foundation` (v0.3.2) and the `objc2` dependency it wraps are Objective-C runtime bindings for Apple platforms. They are freshly added to `rec_aggregation` in this bump. While the PR confirms `cargo build --workspace` succeeds, it would be worth verifying that these crates compile cleanly on the Linux targets used by CI/CD and deployment (they typically provide stub implementations on non-Apple platforms, but cross-platform correctness is worth a quick check).

_{Reviews (1): Last reviewed commit: "chore(deps): bump leanMultisig to f66d4a..." | Re-trigger Greptile}

Thanks for the reviews. Triaged below; pushed dd5f3ca for the one actionable item plus a test.

Addressed (dd5f3ca)

• Incomplete # Errors doc (Greptile P2): aggregate_mixed now notes that raw signatures (not only child proofs) can trigger AggregationError::Aggregation; added # Errors sections to aggregate_signatures and aggregate_proofs.
• Untested new error path (Claude/Kimi): added an #[ignore] (leanVM-backed) test asserting that re-aggregating a valid child proof under a different message returns AggregationError::Aggregation instead of panicking. Passes locally.

Verified non-issues

• "Wire/storage format changed → legacy proofs unreadable" (Codex High docs: add initial roadmap #1): compress/decompress are byte-for-byte identical to the old serialize/deserialize. Both are lz4_flex::compress_prepend_size(postcard::to_allocvec(..)) / its inverse; the rename routes through a new lz4_postcard_encode/decode helper with the same body. No format change, no migration needed.
• "ByteListMiB bound now on compressed bytes → DoS" (Codex High chore: initial repo setup #2): not a regression. The old serialize() was also lz4-compressed, so the 1 MiB cap was always checked against the compressed representation. Behavior is unchanged by this PR.
• objc2/objc2-foundation on Linux (Greptile/Kimi/Claude): target-gated in rec_aggregation under [target.'cfg(target_os = "macos")'.dependencies]. They appear in Cargo.lock (which is platform-agnostic) but are not compiled on Linux CI/deploy targets.
• zk-alloc global allocator (Claude): ethlambda installs tikv_jemallocator::Jemalloc as its #[global_allocator] (bin/ethlambda/src/main.rs). zk-alloc's ZkAllocator is opt-in (#[global_allocator] in the consuming binary) and is never installed here.
• num-bigint "downgrade" 0.4.6 → 0.3.3 (Kimi): both versions coexist in the lock for different upstream sub-crates (mt-field/mt-koala-bear pull 0.3.3; 0.4.6 remains for others). Nothing removed, and it's a transitive choice inside leanMultisig, not under ethlambda's control.

All four #[ignore] crypto integration tests (aggregate single/multiple, wrong message, wrong slot) plus the new error-path test pass against f66d4a9.